protocol: split async and blocking APIs for fetching refmaps #2274
Conversation
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
Thanks a lot for keeping these PR coming, it's much appreciated!
Regarding the CI failure, it can be reproduced with cargo test -p gix-protocol --features blocking-client.
There are a few more general questions that arose from a first relatively brief look.
| None => match delegate.action() { | ||
| Ok(RefsAction::Skip) => Vec::new(), | ||
| Ok(RefsAction::Continue) => { | ||
| #[cfg(feature = "async-client")] |
There was a problem hiding this comment.
It would probably be nice if the test would fail at compile time with a clear message if both features are enabled. Now it would probably fail more indirectly due to invalid code.
There was a problem hiding this comment.
There's still a compile_error!() in the gix-protocol crate root that requires the client features to be mutually exclusive, which will trigger before anything here. So it seems okay for now?
|
|
||
| #[cfg(feature = "blocking-network-client")] | ||
| let ref_map = fetch_refmap.fetch_blocking(progress, &mut self.transport.inner, self.trace)?; | ||
|
|
There was a problem hiding this comment.
This is interesting - now it will compile with both features toggled on, but also yield a warning about the first ref_map never being read from.
Probably that wouldn't compile with both features toggled on, and even though it's not the time to have a test for that just yet (unless it should be expected to fail), I think it's a good time to figure out what should happen in this case.
One thing is for sure: #[gix_protocol::maybe_async::maybe_async] decides if the function is async, and that decides if await is possible at all. Only if it is possible, it should be used, and the blocking version then isn't necessary anymore.
The question is if it is easy enough to 'sync' with what the parent macro does.
It's totally conceivable as well to bring maybe-async like functionality into gix-macros and make it work as we need it.
Maybe you have plans as well that you could share?
There was a problem hiding this comment.
Given that the gix crate's async-network-client and blocking-network-client enable the relevant upstream features in gix-protocol, the gix crate efffectively cannot be compiled with both features enabled anyway. So for now, this is not an issue, and I'd like to defer looking into it in more detail -- I suspect it will be something like the refactoring in this PR: isolate the network calls in a relatively small function and duplicate that part into explicit async and blocking API.
|
Tests should be passing now. |
|
Thanks a lot, I will take a look soon and see to merging it. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the gix-protocol crate to split async and blocking APIs for fetching refmaps, removing reliance on the maybe_async macro in favor of explicit method variants. The change introduces a builder pattern with LsRefsCommand and a FetchRefMap enum that defers execution until either fetch_async or fetch_blocking is called.
Key Changes
- Introduced
LsRefsCommandstruct with separateinvoke_asyncandinvoke_blockingmethods - Created
FetchRefMapenum to represent either a fetched refmap or a command awaiting execution - Updated
Handshake::fetch_or_extract_refmapto returnFetchRefMapinstead of directly fetching - Removed the old
ls_refs()function andActionenum in favor of new API structure
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| gix-protocol/src/ls_refs.rs | Replaced ls_refs() function with LsRefsCommand builder pattern, adding separate invoke_async and invoke_blocking methods |
| gix-protocol/src/handshake/mod.rs | Added FetchRefMap enum and refactored fetch_or_extract_refmap to defer execution with separate async/blocking fetch methods |
| gix-protocol/src/lib.rs | Updated public API exports from ls_refs function to LsRefsCommand |
| gix-protocol/src/fetch/refmap/init.rs | Removed RefMap::fetch() method and associated helper functions, moved logic to LsRefsCommand |
| gix-protocol/src/fetch/types.rs | Updated documentation references to point to new API |
| gix-protocol/src/fetch/negotiate.rs | Updated documentation references to point to new API |
| gix/src/remote/connection/ref_map.rs | Updated to use new two-step API with feature-gated fetch_async/fetch_blocking calls |
| gitoxide-core/src/pack/receive.rs | Updated to use new two-step API with feature-gated fetch_async/fetch_blocking calls |
| gix-protocol/tests/protocol/fetch/_impl.rs | Updated test delegate trait to use simplified action() method and local RefsAction enum, adapted fetch logic to use LsRefsCommand |
| gix-protocol/tests/protocol/fetch/mod.rs | Removed unused imports related to old ls_refs API |
| gix-protocol/tests/protocol/fetch/v2.rs | Updated error matching to reflect flatter error structure |
|
AI code review comments were pretty good, addressed all of them. |
…tch_or_extract_refmap()
2 participants
This is just one part of the problem in gix-protocol, but I'm happy that I was able to resolve it with only about +20 lines despite some duplication.
Happy to take feedback if you can articulate it (to save you on refactoring time).